Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Truncate check length of append #33

Merged
merged 2 commits into from
Oct 29, 2015

Conversation

smeeckaert
Copy link

There is a bug when the size to troncate + the size of the append is the same as the string length.

This cause even a more bigger issue because it doesn't check for a breakpoint since there are no more space in the string thus abruptly cuting the string.

$textShort = 'Short text';
$this->truncate($textShort, 7, '...'); // Expecting: "Short text" (len: 10) return "Short t..."

@@ -33,7 +33,7 @@ public function __construct($text, $charactersCount, $append = '')

public function __toString()
{
if ($this->charactersCount < 0 || strlen($this->text) <= $this->charactersCount) {
if ($this->charactersCount < 0 || strlen($this->text) <= ($this->charactersCount + mb_strlen($this->append))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should switch to mb_strlen everywhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just find out that we have more places like that, I gonna merge it now and prepare separated PR for switching to mb_* functions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a good idea, i've had quite a bit of problems with multibyte string being french where we have stange characters everywhere ! You should add some test in strange charset like arabic/persian (علی) (see: http://stackoverflow.com/questions/25599981/how-can-i-use-strlen-in-php-for-persian)

norberttech pushed a commit that referenced this pull request Oct 29, 2015
@norberttech norberttech merged commit 4f58615 into coduo:master Oct 29, 2015
@norberttech
Copy link
Member

@smeeckaert nice catch! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants